Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ansible doc formats #71070

Merged
merged 2 commits into from Aug 5, 2020
Merged

Conversation

abadger
Copy link
Contributor

@abadger abadger commented Aug 3, 2020

SUMMARY

The ansible specific formats that ansible-doc knows about needed to be updated for things that were added to the website build.

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

@samdoran

@abadger abadger requested a review from samdoran August 3, 2020 16:43
@ansibot ansibot added affects_2.11 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Aug 3, 2020
@ansibot
Copy link
Contributor

ansibot commented Aug 3, 2020

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/cli/__init__.py:49:5: E266: too many leading '#' for block comment

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Aug 3, 2020
Copy link
Contributor

@samdoran samdoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. It would be nice to have some tests. I don't see any existing tests for these reformatting expressions.

@samdoran samdoran added the ci_verified Changes made in this PR are causing tests to fail. label Aug 3, 2020
@samccann
Copy link
Contributor

samccann commented Aug 3, 2020

Are B() and HORIZONTALLINE supported by the docsite? If so, can you add that to this doc - dev_guide/developing_modules_documenting.rst to make it complete in both places?

@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Aug 4, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 4, 2020
@abadger
Copy link
Contributor Author

abadger commented Aug 4, 2020

Unit test added and some bugs that I found while adding tests cases fixed. I'm working on adding documentation that @samccann asked for now as these all come from the code that's currently building the docsite.

Copy link
Contributor

@samccann samccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs portion LGTM

@ansibot ansibot added docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. shipit This PR is ready to be merged by Core and removed core_review In order to be merged, this PR must follow the core review workflow. labels Aug 4, 2020
@ansibot
Copy link
Contributor

ansibot commented Aug 4, 2020

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/cli/doc.py:75:5: E266: too many leading '#' for block comment

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed shipit This PR is ready to be merged by Core labels Aug 4, 2020
* Move tty_ify() and supporting attributes to the DocCLI class as that's
  the only thing using it.
* Add unittest for the code.
* Fix a bug where the substitution macros can be detected when they are
  a part of another word.
* Add support for L(), R(), and HORIZONTALLINE which were added to the
  website docs many years ago.
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Aug 4, 2020
@abadger
Copy link
Contributor Author

abadger commented Aug 5, 2020

@mattclay can you look at the test failures here? It looks to me like it's a false positive. The tests have completed successfully but something (I can't tell if it's in pytest or in ansible-test) is comparing a list of the tests that are supposed to have run and it finds the lists different. However, the difference is actually just in the order of the tests, not in the tests or the results. I'm not sure if there's something I should do (Maybe sorting of the dictionary keys?) or if this is a bug in the test framework that should be solved in the framework.

test/units/cli/test_doc.py Outdated Show resolved Hide resolved
Co-authored-by: Matt Clay <matt@mystile.com>
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Aug 5, 2020
There are also some macros which do not create links but we use them to display certain types of
content in a uniform way:

* ``I()`` for option names. For example: ``Required if I(state=present).`` This is italicized in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought parameters should use C() so they are formatted as inline code. That's just my personal preference, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh oh, lots of things for you to correct, then ;-)

def tty_ify(cls, text):

t = cls._ITALIC.sub(r"`\1'", text) # I(word) => `word'
t = cls._BOLD.sub(r"*\1*", t) # B(word) => *word*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not matter because something else may handle the RST --> HTML conversion, but for in RST, **word** is the syntax for bold while *word* is the syntax for italic. If this is only for the display of ansible-doc, this should be fine. But it'd be surprising if B(word) ended up being italic in the final HTML.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't rst.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rst and html (for building the web docs) are handled by rst_ify and html_ify from this code: https://github.com/ansible-community/antsibull/blob/main/antsibull/jinja2/filters.py

The code in this PR is only for ansible-doc to output to the screen.

This split is why I added the note that the code in both places needs to be changed if new macros are added.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok. I figured they were separate and this was only for ansible-doc output. I just wanted to make sure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun times with cross project relationships. 😁

@abadger abadger merged commit fb144c4 into ansible:devel Aug 5, 2020
@abadger abadger deleted the update-ansible-doc-formats branch August 5, 2020 17:53
abadger added a commit to abadger/ansible that referenced this pull request Aug 5, 2020
* Fix tty_ify bugs and refactor

* Move tty_ify() and supporting attributes to the DocCLI class as that's
  the only thing using it.
* Add unittest for the code.
* Fix a bug where the substitution macros can be detected when they are
  a part of another word.
* Add support for L(), R(), and HORIZONTALLINE which were added to the
  website docs many years ago.

* Update test/units/cli/test_doc.py

Co-authored-by: Matt Clay <matt@mystile.com>

Co-authored-by: Matt Clay <matt@mystile.com>
(cherry picked from commit fb144c4)

Co-authored-by: Toshio Kuratomi <a.badger@gmail.com>
zoredache pushed a commit to zoredache/ansible that referenced this pull request Aug 10, 2020
* Fix tty_ify bugs and refactor

* Move tty_ify() and supporting attributes to the DocCLI class as that's
  the only thing using it.
* Add unittest for the code.
* Fix a bug where the substitution macros can be detected when they are
  a part of another word.
* Add support for L(), R(), and HORIZONTALLINE which were added to the
  website docs many years ago.

* Update test/units/cli/test_doc.py

Co-authored-by: Matt Clay <matt@mystile.com>

Co-authored-by: Matt Clay <matt@mystile.com>
relrod pushed a commit that referenced this pull request Aug 27, 2020
* Fix tty_ify bugs and refactor

* Move tty_ify() and supporting attributes to the DocCLI class as that's
  the only thing using it.
* Add unittest for the code.
* Fix a bug where the substitution macros can be detected when they are
  a part of another word.
* Add support for L(), R(), and HORIZONTALLINE which were added to the
  website docs many years ago.

* Update test/units/cli/test_doc.py

Co-authored-by: Matt Clay <matt@mystile.com>

Co-authored-by: Matt Clay <matt@mystile.com>
(cherry picked from commit fb144c4)

Co-authored-by: Toshio Kuratomi <a.badger@gmail.com>
@ansible ansible locked and limited conversation to collaborators Sep 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.11 bug This issue/PR relates to a bug. docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants